Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Credential Handling Improvements #205

Merged

Conversation

tobias-hashicorp
Copy link
Contributor

@tobias-hashicorp tobias-hashicorp commented Oct 26, 2023

🛠️ Description

All credentials (login, service-principal and workload identity provider) are now cached.

The cache file has moved to creds-cache.json to not interfere with applications that rely on the previous cache file
structure.

It is further now possible to enforce an interactive login (via a configuration flag), which can be used to implement a
cli login functionality.

There are some minor breaking changes:

  • ErrorNoLocalCredsFound is not returned anymore
  • WithSession is not supported anymore

👍 Definition of Done

  • Tests added?
  • Docs updated?

…nt creds

The client credentials were previously discarded and client credentials
configured via the configuration file where never used.
To stay consistent with the naming in the UI. This shouldn't be a
breaking change as the file based client credential configuration was
previously broken anyways.
This will allow to cache user, service principal and workload identity
provider tokens in the same way. It will also provide a common way to
refresh and fetch new tokens if the cached token is expired.

This new cache will replace the user specific session and cache solution
that is currently used.
The config value was only used in that function and constructing the
config inside of the function makes it easier to understand and reason
about.
…Source

It will be necessary to have access to the configuration to be able to
cache the resulting tokens with the identity provider's resource name.
This enables caching for service-principal and workload identity
provider tokens and uses the same caching mechanism for all types of
credentials.
The functionality is now implemented by the common cache.
Without this the cache could keep collecting expired tokens for service
principals and workload identity providers.
With the switch to the common cache it is not possible to return this
typed error anymore. The error does however not seem to be used anywhere
currently and this change should not be breaking.
This option can be used by CLIs to implement a `login` command that will
force a browser login (or in the future client credentials based login)
to occur. The tokens received during the login will be cached for future
invocations.
This makes it easier to understand how the HCP config is used in the
http client and allows to play around with different HCP config options
in the example code.
The previous name hcp-sdk-go-client could be interpreted as it being
a usable client.
Without this it wouldn't be possible to test the token source logic.
To make it easier to find the logic and to make sure that it works as
intended.
@tobias-hashicorp tobias-hashicorp changed the title Tobias/plat 1424/credential handling improvements Credential Handling Improvements Oct 26, 2023
@tobias-hashicorp tobias-hashicorp force-pushed the tobias/plat-1424/credential-handling-improvements branch 2 times, most recently from 010b37d to 74903bf Compare October 26, 2023 20:08
They were not needed anymore and the linter was complaining.
@tobias-hashicorp tobias-hashicorp force-pushed the tobias/plat-1424/credential-handling-improvements branch from 74903bf to 249a701 Compare October 26, 2023 20:11
Copy link
Contributor

@dadgar dadgar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fantastic!

auth/browser.go Outdated Show resolved Hide resolved
// SecretID is the secret id of an HCP Service Principal
SecretID string `json:"secret_id,omitempty"`
// ClientSecret is the client secret of an HCP Service Principal
ClientSecret string `json:"client_secret,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@marianoasselborn Just a heads up the credential file format will change a bit b/c of this

auth/tokencache/cache.go Outdated Show resolved Hide resolved
auth/tokencache/cache.go Show resolved Hide resolved
// Read the cache information from the file, if it exists
cachedTokens, err := readCache(source.cacheFile)
if err != nil {
log.Println(err)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we special case no file existing and not log and on other errors should it delete the cache?

Also is the right way to log, should we pass a logger through the entire Config?

Was going to comment this but NVM, I see that it isn't plumbed that way yet. Future enhancement

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added a check for "NoExist".

I don't think the cache needs to be explicitly deleted as it will automatically overwrite the existing file on the write operation.

auth/tokencache/tokensource.go Outdated Show resolved Hide resolved
auth/tokencache/tokensource.go Outdated Show resolved Hide resolved
log.Printf("failed to refresh the token: %s\n", err)
}

if source.oauthTokenSource != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be preferred to the raw oauthConfig?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. It's intentional that it is this way around. The oauthConfig will perform a refresh, the pure token source will not be able to do so as it does not know the cached refresh token. I have added a comment and modified the code a bit to make this more prominent.

config/tokensource.go Outdated Show resolved Hide resolved
config/tokensource.go Outdated Show resolved Hide resolved
As it is the main interface for the browser login functionality.
@tobias-hashicorp tobias-hashicorp force-pushed the tobias/plat-1424/credential-handling-improvements branch from cdd1961 to 351ddd1 Compare October 27, 2023 11:31
This makes it easier to create folder and files in a consistent way.
It previously tried to refresh a token even if it didn't have a
RefreshToken field. This change should also make it more clear why the
oauth config takes precedence over the token source.
The function does a lot, but the goal is to have a valid token. Evaluate
is quite generic and getValidToken should be more expressive.
The timeout was previously unset and the refresh would have timed out
right away.
Without this the token source could return tokens that are very close to
expiry.
Having no cache file is the default initial state and it is not
problematic and doesn't need to be logged.
It could otherwise be assumed that the expired entries will be removed
from the cache file.
It was initially planned to support service-principal login, but since
that is not implemented right now the distinction between source types
can rely on the sourceType and the complexity can be spared. In the
future a flag might need to be added to differencate between user and sp
based login.
The documentation was not consistent with the current implementatio
anymore.
To make it more consistent with the real implementation.
@tobias-hashicorp tobias-hashicorp force-pushed the tobias/plat-1424/credential-handling-improvements branch from f3199f7 to 8767256 Compare October 27, 2023 11:44
The file would otherwise only get updated when a new token needs to get
cached.
@tobias-hashicorp tobias-hashicorp marked this pull request as ready for review October 27, 2023 12:14
This is needed to be able to test the library without the need for a
real token exchange.

// Garbage collect expired tokens
cachedTokens.removeExpiredTokens()
_ = cachedTokens.write(source.cacheFile)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we not want to log in case the file permissions are wrong, so the user sees the issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't worried about this particular operation failing as it would just leave the expired tokens around and writing of new tokens failing should still be sufficient signal to know that writing is not possible.

@tobias-hashicorp tobias-hashicorp merged commit aa8cd4c into main Oct 27, 2023
3 checks passed
@tobias-hashicorp tobias-hashicorp deleted the tobias/plat-1424/credential-handling-improvements branch October 27, 2023 17:17
@tobias-hashicorp tobias-hashicorp mentioned this pull request Nov 6, 2023
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants